Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Create AP_RCProtocol_SITL #26366

Closed
wants to merge 1 commit into from

Conversation

peterbarker
Copy link
Contributor

This is a WIP.

Fundamental idea is to stop assuming the packets we receive on 5501 are 8 or 16 packed 16-bit quantities, instead feed the received bytes into AP_RCProtocol and let the normal parsing work.

It works as far as being able to get RC input in via the new backend when using sim_vehicle.py.

Some issues arising:

  • many of the backends will not bother looking at the data unless it comes in at a recognised baudrate. There is no baudrate for data received via a UART!
  • we're currently expecting datagrams, not a serial stream on the SITL port
    • this will also make it difficult to return RC telemetry
  • the name isn't right if we want to support Linux's equivalent RC-in
  • RC-input-from-simulator is messy, both in old and new code. Should we have a separate backend for it? The current mix-and-match approach with external simulator input and normal input is messy!
  • in normal sim_vehicle.py you have to start frames coming into the autopilot with rc 3 1000. Existing SITL is weird in that it provides default values for different vehicles.
  • simulated failsafes need fixing
  • perhaps a SIM_RC_XLATE, which, if set, assumes the packed-16-bit format coming in and translates to the specified protocol?

@davidbuzz
Copy link
Collaborator

davidbuzz commented Mar 2, 2024

I had a bit of a look at the code, and i like idea as a concept... but maybe if you are making changes in this area we should stop just throwing 16byte chunks or 32byte chunks over "the wire" without any header/footer/checksum .. or at this early point, start-allowing-both..? ..the only reason it works so well right up-to this point is that sitl/ethernet/udp is so-close to lossless that it might as well be ... but it is a UDP/datagram, and that makes it officially "lossy".

I can imagine it becoming substantially more lossy when running over wifi, or when some part of it is running on an underwhelming bit of hardware , or both ( an esp32 sending/recieving rc sitl packets over wifi for hardware-in-the-loop that is configured to use a "real" RC ?)..?

I'd also like to point out that in my hypothetical implementation that has some-sort-of header bytes, and some-sort-of-checksum.... is that suddenly just a mavlink packet optionally being used for this? I suspect you'd want to support either, and autodetect to fallback to the current behaviour..?

i know, i know its scope creep, so totally unneeded, but the idea occurred to me, so was worth thinking about

@peterbarker
Copy link
Contributor Author

I had a bit of a look at the code, and i like idea as a concept... but maybe if you are making changes in this area we should stop just throwing 16byte chunks or 32byte chunks over "the wire" without any header/footer/checksum .. or at this early point, start-allowing-both..? ..the only reason it works so well right up-to this point is that sitl/ethernet/udp is so-close to lossless that it might as well be ... but it is a UDP/datagram, and that makes it officially "lossy".

I can imagine it becoming substantially more lossy when running over wifi, or when some part of it is running on an underwhelming bit of hardware , or both ( an esp32 sending/recieving rc sitl packets over wifi for hardware-in-the-loop that is configured to use a "real" RC ?)..?

I'd also like to point out that in my hypothetical implementation that has some-sort-of header bytes, and some-sort-of-checksum.... is that suddenly just a mavlink packet optionally being used for this? I suspect you'd want to support either, and autodetect to fallback to the current behaviour..?

i know, i know its scope creep, so totally unneeded, but the idea occurred to me, so was worth thinking about

I hope this is structured in a way that you can actually just feed any protocol in on that serial port and it will be parsed out by the appropriate backend. I haven't tested that yet! This is why I mentioned the "baud rate" thing - if we want to be able to parse out CRSF we'd currently need to convince the CRSF backend that this really was 115200 serial!

To your wider point - raw packed 16-bit quantities - I don't think we can get rid of it any time soon. Too much else out there may rely on it.

Note that we could do this very differently, by having the AP_RCProtocol_SITL backend do all of the port binding itself, rather than have the HAL do it. This would let Linux reuse it rather more easily. We'd then need to find some other way to take input via other protocols - bit I believe we may already be able to do that by simply setting a serial port to listen on a network socket and setting the protocol to 23... tests byte-protocols but not pulse-stream protocols...

@peterbarker peterbarker closed this Apr 1, 2024
@peterbarker peterbarker deleted the pr/sitl-rc-in branch April 1, 2024 00:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants